Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update DFG::combineForIter() #21

Merged
merged 15 commits into from
Jun 13, 2024
Merged

Update DFG::combineForIter() #21

merged 15 commits into from
Jun 13, 2024

Conversation

MeowMJ
Copy link
Collaborator

@MeowMJ MeowMJ commented Apr 24, 2024

I find two bugs in combineForIter() when I am debugging combineForUnroll() (because they share key codes).

  1. isSuccessor() is used incorrectly. succNode->isSuccessor(headNode) means to tell whether succNode is a successor of headNode. But we actually want to find whether headNode is a successor of succNode to guarantee it is a cycle path.
  2. currentFunc doesn't recover its original position when we finished finding the target pattern under specific dfgNode.

@MeowMJ
Copy link
Collaborator Author

MeowMJ commented Apr 25, 2024

I finished the DFG::combineForUnroll(). One problem, I can't new a DFG node by my code. So I'm not sure if the result fits your idea.

conv (unroll twice) before combineForUnroll: (0)phi->(11)add->(20)add->(0)phi
conv-Unroll2-before

conv (unroll twice) after combineForUnroll: (0)phi<->(11)add (11)add->(20)add
conv-Unroll2-after

@tancheng
Copy link
Owner

I didn't get the fusion about 0, 11, 20 DFG nodes. Can we have a sync on this?

src/DFG.h Outdated Show resolved Hide resolved
src/DFG.cpp Outdated Show resolved Hide resolved
src/DFG.h Outdated Show resolved Hide resolved
src/DFGNode.cpp Outdated Show resolved Hide resolved
src/DFGNode.cpp Outdated Show resolved Hide resolved
src/DFG.cpp Show resolved Hide resolved
@MeowMJ
Copy link
Collaborator Author

MeowMJ commented Jun 9, 2024

I have finished this pull request. Below is an example.

conv.c unroll 2 times before combineForUnroll()
convU2

conv.c unroll 2 times after combineForUnroll()
convU2-NEW

src/DFG.cpp Outdated Show resolved Hide resolved
@tancheng
Copy link
Owner

Can this currently be used as a parameter/argument set inside the param.json?

@tancheng
Copy link
Owner

And what about unrolling factor = 3?

@tancheng
Copy link
Owner

And can you also provide a fused DFG that both the unrolling and the i++ ctrl flow are fused?

@tancheng
Copy link
Owner

I didn't see my previous comments are addressed?

@MeowMJ
Copy link
Collaborator Author

MeowMJ commented Jun 10, 2024

Can this currently be used as a parameter/argument set inside the param.json?

No. Below is an example using it (in DFG.cpp).

    list<string>* toConbinePattern = new list<string>[5];
    toConbinePattern->push_back("phi");
    toConbinePattern->push_back("add");
    toConbinePattern->push_back("add");
    toConbinePattern->push_back("add");
    toConbinePattern->push_back("add");
    combineForUnroll(toConbinePattern);

@MeowMJ
Copy link
Collaborator Author

MeowMJ commented Jun 10, 2024

And what about unrolling factor = 3?

And what about unrolling factor = 3?

conv.c unroll = 3 before conbineForUnroll()
convU3

conv.c unroll = 3 after conbineForUnroll()
convU3-NEW

@MeowMJ
Copy link
Collaborator Author

MeowMJ commented Jun 10, 2024

And can you also provide a fused DFG that both the unrolling and the i++ ctrl flow are fused?

conv.c unroll=2 after fuse and combineForIter() and combineForUnroll()
convU2-ALL

@tancheng
Copy link
Owner

For the conv.c unroll = 3 before combineForUnroll(), are we able to fuse 3 nodes out of the 4? Say fusing 28/0/11 without touching 22? There will be still one cycle with length of 2 after your merging, but would relief HW complexity.

And do you know why the edge from 25 to 0 changed its color from blue (ctrl) to red (data)? Can this be fixed?

@MeowMJ
Copy link
Collaborator Author

MeowMJ commented Jun 10, 2024

This is conv.c unroll = 3 fuse 3 nodes
convU3-0610

I am trying to figure out why the control edge becomes a data edge.

@MeowMJ
Copy link
Collaborator Author

MeowMJ commented Jun 12, 2024

For the conv.c unroll = 3 before combineForUnroll(), are we able to fuse 3 nodes out of the 4? Say fusing 28/0/11 without touching 22? There will be still one cycle with length of 2 after your merging, but would relief HW complexity.

And do you know why the edge from 25 to 0 changed its color from blue (ctrl) to red (data)? Can this be fixed?

If we want to keep the blue edge, we should guarantee that the nodes of the blue edge are PatternRoot. Otherwise, the blue edge will be replaced by a red edge. See https://github.com/tancheng/CGRA-Mapper/blob/master/src/DFG.cpp#L76

To keep the blue edge, we may need to modify the replaceDFGEdge().

conv.c Unroll = 3 choose phi-add-add-add to fuse
convU3-NEW

conv.c Unroll = 3 choose add-phi-add-add to fuse
convU3-0611

@tancheng
Copy link
Owner

ha, so for now, one ctrl edge out of the two edges will become data edge. Then do you think you can fix this in this PR?

@MeowMJ
Copy link
Collaborator Author

MeowMJ commented Jun 12, 2024 via email

@tancheng
Copy link
Owner

Thanks!

@MeowMJ
Copy link
Collaborator Author

MeowMJ commented Jun 13, 2024

I changed replaceDFGEdge() to make sure that every edge being replaced can keep their ctrl property.

conv.c unroll=3 after combineForUnroll()
convU3-0612

src/DFG.cpp Show resolved Hide resolved
@tancheng tancheng merged commit c408b66 into tancheng:master Jun 13, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants